Add GRPC + TLS integration tests (notably, test important security properties) by matthild · Pull Request #1327 · grpc/grpc-java

matthild · 2016-01-19T16:42:05Z

This pull request adds end-to-end tests of the following basic functionality:

A server configured to require client authentication refuses to accept connections from a client that has an untrusted certificate.
A server configured to require client authentication actually does require client authentication.
A client and a server configured using GrpcSslContexts can successfully communicate with each other. (This last test fails -- any idea why? For now I've annotated it @Ignore to avoid breaking the build.)

These tests work by having an actual GRPC client talk through the operating system's network stack to an actual GRPC server listening on a port, and thus mimic how GRPC will behave in real-world applications.

(I have also drafted tests of some other important security properties, but suggest adding them later via a separate pull request, once these initial tests are in place. The additional tests will be easier to add in a sensible way if some organization of the test certificates directory happens first.)

grpc-kokoro · 2016-01-19T16:42:06Z

Can one of the admins verify this patch?

googlebot · 2016-01-19T16:42:07Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

The Travis build failed without it, and claimed that committing generated files is necessary. https://travis-ci.org/grpc/grpc-java/jobs/103388675

matthild · 2016-01-20T13:16:55Z

That Travis CI build failed with an indication that interop-testing/src/generated/main/grpc/io/grpc/testing/integration/EchoServiceGrpc.java has been "modified" -- i.e., that the version of that file generated during the Travis build is different than the version that's in the repository. However, the version that's in the repository is bit-for-bit identical to the version that gradlew produces during a build, as shown below.

$ rm interop-testing/src/generated/main/grpc/io/grpc/testing/integration/EchoServiceGrpc.java

$ git status
On branch matthild_tls_client_auth_test_v3
Your branch is up-to-date with 'origin/matthild_tls_client_auth_test_v3'.
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

    deleted:    interop-testing/src/generated/main/grpc/io/grpc/testing/integration/EchoServiceGrpc.java

no changes added to commit (use "git add" and/or "git commit -a")

$ ./gradlew build
# reports "BUILD SUCCESSFUL"

$ git status
On branch matthild_tls_client_auth_test_v3
Your branch is up-to-date with 'origin/matthild_tls_client_auth_test_v3'.
nothing to commit, working directory clean

Using ./gradlew assemble generateTestProto (which the Travis build uses) instead of ./gradlew build yields the same result.

Is there some trick to adding a new .proto file?

I'm running Mac OS X (El Capitan) and have protoc 3.0.0-beta1 installed (built locally).

$ protoc --version
libprotoc 3.0.0

ejona86 · 2016-01-20T21:01:27Z

@matthild, the problem with EchoServiceGrpc.java confused me a lot too. I figured it out though: Travis tests the merged form of the change, so your generated file is lacking the changes from my recent commit d52429d. Rebasing on current master and regenerating EchoServiceGrpc.java should resolve the problem.

Thanks for working through all you have. I know it is a pain to get the codegen plugin compiling because of dependencies.

I'm thinking of ways that the error could have been a little more helpful. Maybe if it did git diff it would be more obvious that something unrelated had to have been at play.

ejona86 · 2016-01-20T21:05:52Z

@nmittler, this surprises me, mainly because I thought we already had the netty_tcnative dependency in place here. It seems we are including tcnative in the binaries (via the distributions section below) but was using Jetty ALPN during gradle build. Do you remember if we did not use tcnative here before because of concerns about needing OpenSSL installed, or was it something different?

IIRC, that was the outcome of our previous discussions ... that we wouldn't include tcnative in the automated tests due to the OpenSSL dependency. Not that configuring jetty-alpn is easier.

The GRPC docs recommend using OpenSSL. Are there any plans to have integration tests that use this recommended configuration?

The difference is significant. For example, removing libraries.netty_tcnative from build.gradle caused the new test annotated with @Ignore to begin passing -- i.e., that test appears to fail with OpenSSL but pass with Jetty ALPN. Additionally, there is a gaping security problem that appears to occur with OpenSSL but not with Jetty ALPN.

If we run all the tests using Jetty ALPN only, what should become of the following TODO in TlsTest.java? Should I just remove it?

// TODO: Use @RunWith(Parameterized.class) to run these tests for all TLS providers.

For now I've reverted this change (thus, the integration tests run with Jetty ALPN only, not with OpenSSL), although personally I think there really ought to be integration tests that use the recommended configuration.

Just for starters, I do know of other tests that are using OpenSSL: the interop-testing unit tests are run internally at google using tcnative (I just confirmed). Tcnative is also used for the actual interop testing, both internally (I just confirmed) and externally (granted, those are a different sort of test).

I think the original plan was to use tcnative on Travis (because otherwise, why do a99e977?), but still allow small-time contributors to use Jetty ALPN. I know we hit a snag with OkHttp, because it only supports Jetty ALPN on OpenJDK and it couldn't talk to Netty on Linux because OpenSSL is generally too old to include ALPN. That's a different problem than on OS X and Windows, because on those platforms OpenSSL is generally not available and the code falls back to Jetty ALPN.

So for Linux we needed to check whether OpenSSL is new enough. But that was broken. I think once it was fixed we simply failed to follow through. Time to follow through; I've sent out #1332.

ejona86 · 2016-01-20T21:42:59Z

I just filed #1330 for what I expect to be the same problem as here.

Thus, tests run using Jetty ALPN only, not using OpenSSL.

ejona86 · 2016-01-21T21:35:03Z

@matthild LGTM. I'll merge it and then close the PR

ejona86 · 2016-01-21T21:50:38Z

Cherry-picked as 95d7bfd. Thank you very much! That was quite involved.

With #1332 we hopefully are able to reliably trigger the problem, and investigate what is going wrong.

Add some initial integration tests for GRPC's TLS support.

07a0a93

Check in generated code.

7525b99

The Travis build failed without it, and claimed that committing generated files is necessary. https://travis-ci.org/grpc/grpc-java/jobs/103388675

ejona86 reviewed Jan 20, 2016
View reviewed changes

ejona86 mentioned this pull request Jan 20, 2016

Receiving ClosedChannelException instead of actual error #1330

Closed

ejona86 reviewed Jan 20, 2016
View reviewed changes

Use existing server1.pem cert instead of creating localhost_server.pem.

4b2115f

matthild mentioned this pull request Jan 21, 2016

GRPC with TLS mutual authentication: Server accepts untrusted client certificate #1325

Closed

matthild added 3 commits January 21, 2016 14:15

Remove unnecessary code to make client trust its own cert.

1aac04b

Use TestService.emptyCall instead of introducing a new EchoService.

e9b368e

Run integration tests without Netty tcnative.

057297d

Thus, tests run using Jetty ALPN only, not using OpenSSL.

ejona86 closed this Jan 22, 2016

matthild mentioned this pull request Feb 29, 2016

Test that a GRPC client rejects untrusted server certs. #1498

Closed

lock Bot locked as resolved and limited conversation to collaborators Jan 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add GRPC + TLS integration tests (notably, test important security properties) - #1327

Add GRPC + TLS integration tests (notably, test important security properties)#1327
matthild wants to merge 6 commits into
grpc:masterfrom
matthild:matthild_tls_client_auth_test_v3

matthild commented Jan 19, 2016

Uh oh!

grpc-kokoro commented Jan 19, 2016

Uh oh!

googlebot commented Jan 19, 2016

Uh oh!

matthild commented Jan 20, 2016

Uh oh!

ejona86 commented Jan 20, 2016

Uh oh!

ejona86 Jan 20, 2016

Uh oh!

nmittler Jan 20, 2016

Uh oh!

matthild Jan 21, 2016

Uh oh!

matthild Jan 21, 2016

Uh oh!

ejona86 Jan 21, 2016

Uh oh!

ejona86 Jan 20, 2016

Uh oh!

ejona86 commented Jan 21, 2016

Uh oh!

ejona86 commented Jan 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

matthild commented Jan 19, 2016

Uh oh!

grpc-kokoro commented Jan 19, 2016

Uh oh!

googlebot commented Jan 19, 2016

Uh oh!

matthild commented Jan 20, 2016

Uh oh!

ejona86 commented Jan 20, 2016

Uh oh!

ejona86 Jan 20, 2016

Choose a reason for hiding this comment

Uh oh!

nmittler Jan 20, 2016

Choose a reason for hiding this comment

Uh oh!

matthild Jan 21, 2016

Choose a reason for hiding this comment

Uh oh!

matthild Jan 21, 2016

Choose a reason for hiding this comment

Uh oh!

ejona86 Jan 21, 2016

Choose a reason for hiding this comment

Uh oh!

ejona86 Jan 20, 2016

Choose a reason for hiding this comment

Uh oh!

ejona86 commented Jan 21, 2016

Uh oh!

ejona86 commented Jan 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants